Refactoring compilation engine by unv-unv · Pull Request #300 · consulo/consulo-java

unv-unv · 2026-06-23T21:42:33Z

No description provided.

unv-unv · 2026-06-23T21:42:54Z

@claude review pr with inline comments

github-actions · 2026-06-23T21:43:34Z

Claude finished @unv-unv's task in 3m 0s —— View job

PR Review: Refactoring compilation engine

Gather context (diff, changed files)
Read repo guidelines (no AGENTS.md/CLAUDE.md present)
Review changed files
Post inline review comments

I reviewed all 11 changed files. The bulk of this PR is a clean mechanical reformat (tabs → 4-space indent, final cleanup, diamond operators) plus a genuine API migration from the old callback.message(...) / context.addMessage(...) calls to the new fluent newError/newWarning/newInfo/newMessage(...).…add() MessageBuilder API. That migration is consistent and reads well across OutputParser, CompilerParsingThread, BackendCompilerWrapper, FilePathActionJavac, and both JavacOutputParser variants.

I did find 3 bugs introduced by the migration, posted as inline suggestions you can apply with one click:

#	File	Line	Issue
1	`JavaToolMonitor.java`	191	Infinite recursion → `StackOverflowError`. `MessageBuilderWrapper.url()` calls its own `url(...)` instead of `myDelegate.url(...)`; the resolved `VirtualFile.getUrl()` re-resolves to the same file forever. Every `logInfo/logError/logWarning` carrying a file URI hits this.
2	`JavaCompiler.java`	111	Compiler error swallowed — missing `.add()`. A `CompilerException` from `wrapper.compile(...)` is built but never reported, so a real failure only reaches idea.log and the user sees a silent "success".
3	`JavacOutputParser.java`	163	OOM message dropped — missing `.add()`. The `java.lang.OutOfMemoryError` branch builds an error that is never added.

The common thread for #2 and #3: the fluent API is lazy — building a MessageBuilder without .add() is a no-op, unlike the old addMessage(...) which reported immediately. It's worth grepping for any other new{Error,Warning,Info,Message}( not terminated by .add() (I checked the changed files; these two were the only stragglers, but the pattern is easy to repeat).

Minor notes (no inline comment needed)

OutputParser.java and JavacOutputParser.java end without a trailing newline (\ No newline at end of file). Low priority, but worth fixing while touching these files.
JavaToolMonitor previously fell back to adding a message with a null URL when the file couldn't be resolved; the new wrapper silently skips setting a URL (message still added) — behavior is equivalent, just noting it's intentional.

Nice cleanup overall — once the three .add()/delegation fixes land this is good to go.
· branch refactoring-compilation-engine

github-actions · 2026-06-23T21:46:06Z

-			return true;
-		}
+        if (line.endsWith("java.lang.OutOfMemoryError")) {
+            callback.newError(CompilerLocalize.errorJavacOutOfMemory());


Bug: message is never reported — missing .add().

Every other newError(...)/newInfo(...) call in this file ends with .add(), but here the built MessageBuilder is discarded, so the out-of-memory error is silently dropped. In the pre-refactor code this was addMessage(callback, CompilerMessageCategory.ERROR, ...), which did report it.

Suggested change

callback.newError(CompilerLocalize.errorJavacOutOfMemory());

callback.newError(CompilerLocalize.errorJavacOutOfMemory()).add();

github-actions · 2026-06-23T21:46:14Z

+            wrapper.compile(parsingInfo);
+        }
+        catch (CompilerException e) {
+            context.newError(LocalizeValue.of(ExceptionUtil.getThrowableText(e)));


Bug: compiler error is swallowed — missing .add().

When wrapper.compile(...) throws a CompilerException, the error is built but never added to the context, so the failure is only logged to idea.log and the user sees a "successful" compile with no error message. Previously this path called context.addMessage(CompilerMessageCategory.ERROR, ...).

Suggested change

context.newError(LocalizeValue.of(ExceptionUtil.getThrowableText(e)));

context.newError(LocalizeValue.of(ExceptionUtil.getThrowableText(e))).add();

github-actions · 2026-06-23T21:46:22Z

+            try {
+                VirtualFile fileByURL = VirtualFileUtil.findFileByURL(new URI(url).toURL());
+                if (fileByURL != null) {
+                    return url(fileByURL.getUrl());


Bug: infinite recursion / StackOverflowError. url(...) is the wrapper's own method, not the delegate's. On a successful lookup it re-invokes itself with the resolved VirtualFile.getUrl(), which resolves to the same file again and recurses forever (the message is also never forwarded to the delegate). This should delegate instead:

Suggested change

return url(fileByURL.getUrl());

return myDelegate.url(fileByURL.getUrl());

unv-unv · 2026-06-23T21:53:10Z

@claude review pr with inline comments

github-actions · 2026-06-23T21:53:50Z

Claude finished @unv-unv's task in 4m 2s —— View job

PR Review (re-review): Refactoring compilation engine

Gather context (diff vs origin/master, changed files)
Verify the migration bugs flagged in the previous review
Re-scan all changed files for the lazy-API (new…() without .add()) pattern
Post findings

✅ All three previously-flagged bugs are now fixed (commit `e472cfd32` "Fixes.")

#	File · line	Status
1	`JavaToolMonitor.java:191`	Fixed — now `return myDelegate.url(fileByURL.getUrl());`, no more self-recursion / `StackOverflowError`.
2	`JavaCompiler.java:111`	Fixed — `CompilerException` path now ends with `.add()`, so the error is reported.
3	`JavacOutputParser.java:163`	Fixed — the `OutOfMemoryError` branch now ends with `.add()`.

I re-scanned every new{Error,Warning,Info,Message}(…) call across all 11 changed files: each builder chain is now terminated with .add(). No remaining dropped-message regressions from the fluent-API migration.

Minor note (non-blocking)

OutputParser.java (last line, } at line 96) still has no trailing newline at end of file — the sibling JavacOutputParser.java was given one in this PR, so it's worth matching for consistency. One-line fix: add a final newline.
- _{(I tried to attach this as a one-click inline suggestion, but the CI shell guard blocks the newline-escaping needed to post a suggestion block via gh api; flagging here instead.)}

Verdict

The refactor is a clean mechanical reformat (tabs → 4-space, final/diamond cleanup) plus a consistent migration to the fluent MessageBuilder API, and the lazy-API hazard that caused bugs #2/#3 has been swept across the changed files. With the three fixes landed, this looks good to merge — the only outstanding item is the trivial trailing-newline nit above.
· branch refactoring-compilation-engine

Refactoring compilation engine.

f94d101

github-actions Bot reviewed Jun 23, 2026

View reviewed changes

Fixes.

e472cfd

VISTALL merged commit 421aae8 into consulo:master Jun 23, 2026
1 check passed

unv-unv deleted the refactoring-compilation-engine branch June 24, 2026 08:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactoring compilation engine#300

Refactoring compilation engine#300
VISTALL merged 2 commits into
consulo:masterfrom
unv-unv:refactoring-compilation-engine

unv-unv commented Jun 23, 2026

Uh oh!

unv-unv commented Jun 23, 2026

Uh oh!

github-actions Bot commented Jun 23, 2026 •

edited

Loading

Uh oh!

github-actions Bot Jun 23, 2026

Uh oh!

github-actions Bot Jun 23, 2026

Uh oh!

github-actions Bot Jun 23, 2026

Uh oh!

unv-unv commented Jun 23, 2026

Uh oh!

github-actions Bot commented Jun 23, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	callback.newError(CompilerLocalize.errorJavacOutOfMemory());
	callback.newError(CompilerLocalize.errorJavacOutOfMemory()).add();

	context.newError(LocalizeValue.of(ExceptionUtil.getThrowableText(e)));
	context.newError(LocalizeValue.of(ExceptionUtil.getThrowableText(e))).add();

	return url(fileByURL.getUrl());
	return myDelegate.url(fileByURL.getUrl());

Uh oh!

Conversation

unv-unv commented Jun 23, 2026

Uh oh!

unv-unv commented Jun 23, 2026

Uh oh!

github-actions Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review: Refactoring compilation engine

Minor notes (no inline comment needed)

Uh oh!

github-actions Bot Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

unv-unv commented Jun 23, 2026

Uh oh!

github-actions Bot commented Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review (re-review): Refactoring compilation engine

✅ All three previously-flagged bugs are now fixed (commit e472cfd32 "Fixes.")

Minor note (non-blocking)

Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Jun 23, 2026 •

edited

Loading

github-actions Bot commented Jun 23, 2026 •

edited

Loading

✅ All three previously-flagged bugs are now fixed (commit `e472cfd32` "Fixes.")